Mesh-based Non-collision Constraints #771
Conversation
zhx06
requested review from
alexmillane,
cvolkcvolk,
peterd-NV,
qianl-nv,
viiik-inside and
xyao-nv
as code owners
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — PR #771
Mesh-based Non-collision Constraints
Summary
This PR adds sphere-to-SDF mesh collision support as an alternative to AABB overlap detection. The architecture is clean — CollisionMode.MESH integrates well into the existing NoCollisionLossStrategy dispatch, and the greedy sphere decomposition + Warp SDF kernel approach is sound. The test suite is thorough (542 lines!) with good coverage of dispatch routing, gradient flow, and integration.
Findings
| # | Severity | Finding |
|---|---|---|
| 1 | 🟡 Warning | Validator creates fresh WarpMeshManager per call — cache never reused |
| 2 | 🟡 Warning | Scale applied post-transform in extract_trimesh_from_usd may be incorrect for nested prims |
| 3 | 🔵 Suggestion | object_base.py abstract method has no explicit return None |
| 4 | 🔵 Suggestion | Sentinel warning pattern on function object is not thread-safe |
| 5 | 🔵 Suggestion | Consider documenting the rotated-anchor limitation more prominently |
See inline comments for details.
Update (5e86ed0a): Reviewed incremental changes since 655ac73.
Addressed Findings
- ✅ Finding #1 resolved —
_get_cpu_mesh_manager()now lazily creates and caches theWarpMeshManageron the instance, eliminating redundant allocations per validation call. Good fix. - ✅ Finding #2 resolved — Removed erroneous
.Ttranspose onComputeLocalToWorldTransforminusd_helpers.py. USD returns row-major matrices; the transpose was producing incorrect vertex transforms for nested prims.
Other Changes
- Validation logic refactored (
_validate_placement): Mesh mode now skips AABB validation entirely (elsebranch). Previously both checks ran in mesh mode — the AABB check was redundant and could produce false negatives for non-convex shapes. Clean improvement. - Test suite trimmed: Removed
test_sphere_count_respects_budget,test_cache_key_differs_for_different_meshes,test_dispatch_falls_back_when_obj_is_none, andtest_mesh_zero_loss_separated_cylinders. These removals look intentional (simplified scope / covered elsewhere), though removing cache-key differentiation test reduces regression coverage on the caching layer.
Remaining Observations
- Findings #3–#5 from original review remain unaddressed (low priority, suggestions only).
- The new
_get_cpu_mesh_manageruseshasattrcheck — works fine butOptionalattribute initialized in__init__would be more explicit.
Overall: Good incremental improvement. The two main warnings from the initial review are resolved. No new concerns.
Update (729d892c): Reviewed incremental changes since 5e86ed0a.
Changes in this push (2 files)
-
relation_loss_strategies.py— Addedparent_pos_resolved.expand(batch_size, -1)before the per-batch loop. This fixes a shape mismatch whenparent_pos_resolvedis not already batch-expanded (e.g., single parent broadcast to multiple children). Correct fix. -
warp_mesh_manager.py— Wrappedgetattr(obj, "scale", ...)intuple()for cache key computation. This prevents unhashable types (e.g., numpy arrays or torch tensors returned by.scale) from breaking the dict lookup. Necessary bugfix.
Assessment
Both changes are small, targeted bugfixes. No new concerns introduced. All previous suggestions (#3–#5) remain low-priority and unaddressed.
Greptile SummaryThis PR adds
Confidence Score: 5/5The change is safe to merge. The core mesh-collision pipeline is well-structured, correctly wired for gradient flow in both directions, and covered by a thorough test suite including sentinel failure, diagonal-cylinder acceptance, and multi-env batch correctness. All major bugs flagged in earlier review rounds have been addressed: wrong transpose in USD transform extraction, stale BVH cache per validation call, process-lifetime sentinel flag, batch-size IndexError in mesh loss, unhashable list scale key, missing child-orientation rotation. The mesh-loss path was moved entirely into RelationSolver, the sentinel flag is now per-instance and reset before each pass, _cpu_mesh_manager is cached on ObjectPlacer, and the null-dereference in skip_mesh_pairs is guarded by self._mesh_manager is not None. No new defects were found in the changed code paths. No files require special attention. The new warp_sdf_kernels.py, warp_mesh_manager.py, and the mesh loss path in relation_solver.py are the most complex additions and are all well-tested. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant OP as ObjectPlacer
participant RS as RelationSolver
participant WMM as WarpMeshAndSphereCache
participant SDF as warp_sdf_kernels
OP->>RS: solve(objects, initial_positions, orientations)
RS->>WMM: get_collision_mesh(obj) per object
WMM-->>RS: trimesh or None
RS->>WMM: get_query_spheres(mesh, obj)
WMM-->>RS: (K,4) sphere tensor
RS->>WMM: get_warp_mesh(mesh, obj)
WMM-->>RS: wp.Mesh (BVH)
RS->>RS: _finalize_mesh_cache (forward + reverse)
loop optimizer steps
RS->>RS: _compute_no_overlap_loss()
RS->>SDF: multi_mesh_sdf(active_centers, mesh_ids, indices)
SDF-->>RS: (N,) SDF values + gradients
RS->>RS: "penetration = relu(radii + clearance - sdf)"
RS->>RS: AABB fallback for mesh-less pairs
end
RS-->>OP: final positions
OP->>OP: "_validate_no_overlap(skip_mesh_pairs=True)"
OP->>OP: _validate_no_overlap_mesh(positions, orientations)
OP->>SDF: mesh_sdf(centers_in_target_frame, warp_mesh)
SDF-->>OP: SDF values
OP-->>OP: accept/reject placement
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant OP as ObjectPlacer
participant RS as RelationSolver
participant WMM as WarpMeshAndSphereCache
participant SDF as warp_sdf_kernels
OP->>RS: solve(objects, initial_positions, orientations)
RS->>WMM: get_collision_mesh(obj) per object
WMM-->>RS: trimesh or None
RS->>WMM: get_query_spheres(mesh, obj)
WMM-->>RS: (K,4) sphere tensor
RS->>WMM: get_warp_mesh(mesh, obj)
WMM-->>RS: wp.Mesh (BVH)
RS->>RS: _finalize_mesh_cache (forward + reverse)
loop optimizer steps
RS->>RS: _compute_no_overlap_loss()
RS->>SDF: multi_mesh_sdf(active_centers, mesh_ids, indices)
SDF-->>RS: (N,) SDF values + gradients
RS->>RS: "penetration = relu(radii + clearance - sdf)"
RS->>RS: AABB fallback for mesh-less pairs
end
RS-->>OP: final positions
OP->>OP: "_validate_no_overlap(skip_mesh_pairs=True)"
OP->>OP: _validate_no_overlap_mesh(positions, orientations)
OP->>SDF: mesh_sdf(centers_in_target_frame, warp_mesh)
SDF-->>OP: SDF values
OP-->>OP: accept/reject placement
Reviews (22): Last reviewed commit: "improve test cases, edit docstring" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
zhx06
force-pushed
the
zxiao/feature/mesh_support
branch
from
729d892 to
db06239
Compare
zhx06
force-pushed
the
zxiao/feature/mesh_support
branch
from
ef73a02 to
7c46283
Compare
alexmillane
left a comment
alexmillane
left a comment
There was a problem hiding this comment.
First partial review.
Looks good. I haven't got to the warp mesh based stuff.
| # Rotate child sphere centers by net_yaw = child_yaw - parent_yaw. | ||
| net_yaw = child_yaw - parent_yaw | ||
| if net_yaw != 0.0: | ||
| cos_n = math.cos(net_yaw) | ||
| sin_n = math.sin(net_yaw) | ||
| rx = centers_local[:, 0] * cos_n - centers_local[:, 1] * sin_n | ||
| ry = centers_local[:, 0] * sin_n + centers_local[:, 1] * cos_n | ||
| centers_local = torch.stack([rx, ry, centers_local[:, 2]], dim=-1) | ||
|
|
||
| batch_size = child_pos.shape[0] | ||
| parent_pos_resolved = parent_pos_resolved.expand(batch_size, -1) | ||
| total_loss = torch.zeros(batch_size, device=device, dtype=child_pos.dtype) | ||
|
|
||
| for b in range(batch_size): | ||
| offset = child_pos[b] - parent_pos_resolved[b] | ||
| # Rotate offset into the parent's local frame. | ||
| if parent_yaw != 0.0: | ||
| cos_p = math.cos(-parent_yaw) | ||
| sin_p = math.sin(-parent_yaw) | ||
| ox = offset[0] * cos_p - offset[1] * sin_p | ||
| oy = offset[0] * sin_p + offset[1] * cos_p | ||
| offset = torch.stack([ox, oy, offset[2]]) | ||
| centers_in_parent = centers_local + offset |
There was a problem hiding this comment.
Two suggestions:
- Part of this code represents transforming vectors in one frame to another. Under a transformation (that happens to only have a yaw component). I'm wondering if we can use more general tools for this. For example, can we use:
isaaclab.utils.mathfor examplequat_applyfor applying rotations, rather than recoding the code for rotations here. - It is useful if you use structured notation. For example
q_B_Arepresents the rotation from the frameAto the frameB.
I just wrote a good example of doing this type of this here
| n_candidates = max(num_spheres, n_candidates) | ||
| n_surface = max(n_candidates, n_surface) | ||
|
|
||
| rng = np.random.default_rng(seed) |
There was a problem hiding this comment.
This might destroy the seeding we do elsewhere.
I'm wondering if we should avoid setting the seed anywhere in our codebase, except for the one place we set it in Isaac Lab.
@xyao-nv @peterd-NV What do you think? Should we outlaw setting seeds in our code?
zhx06
force-pushed
the
zxiao/feature/mesh_support
branch
2 times, most recently
from
af4e742 to
bc78db6
Compare
alexmillane
left a comment
alexmillane
left a comment
There was a problem hiding this comment.
Another partial review
zhx06
force-pushed
the
zxiao/feature/mesh_support
branch
2 times, most recently
from
e844ccc to
e307fcd
Compare
| parent_world_bbox=anchor_world_bbox, | ||
| child_obj=child, | ||
| parent_obj=anchor, | ||
| parent_pos=None, | ||
| ) | ||
| if debug: | ||
| print(f" [NoOverlap] {child.name} vs {anchor.name}: loss={loss.mean().item():.6f}") | ||
| total_loss = total_loss + loss | ||
|
|
||
| # Against other non-anchors (unique pairs, both directions) | ||
| for j in range(i + 1, len(non_anchor_objects)): |
There was a problem hiding this comment.
_mesh_manager when skip_mesh_pairs=True
_compute_no_overlap_loss_aabb(skip_mesh_pairs=True) is only reached when collision_mode == MESH and calls self._mesh_manager.get_collision_mesh(child) directly. After __deepcopy__ the custom __deepcopy__ explicitly sets _mesh_manager = None. If the copied solver ever enters _compute_total_loss without first calling solve() (which is the only method that calls _prepare_mesh_collision_cache), this raises AttributeError: 'NoneType' object has no attribute 'get_collision_mesh'. The method should assert self._mesh_manager is not None or add a short-circuit early return before dereferencing it when skip_mesh_pairs=True.
zhx06
force-pushed
the
zxiao/feature/mesh_support
branch
from
e307fcd to
2b8adcd
Compare
| source, | ||
| source_mesh, | ||
| source_pos, | ||
| target, | ||
| target_mesh, | ||
| target_pos, | ||
| mesh_manager, | ||
| tolerance, | ||
| orientations, | ||
| ) -> bool: | ||
| """True if source's spheres penetrate target's mesh.""" |
There was a problem hiding this comment.
When MESH mode is active and an anchor object has (a) no collision mesh and (b) a non-pure-Z rotation (roll or pitch), _pair_aabb_overlaps is reached via the AABB fallback in _validate_no_overlap_mesh. The assert on qx/qy then propagates as an unhandled AssertionError, crashing the entire place() call instead of returning False or raising a descriptive ValueError.
The solver-side guard in _prepare_mesh_collision_cache only fires for anchor pairs that have a mesh; anchors without meshes silently bypass it, so this is the only gate. Replacing the assert with an explicit ValueError (or a return True for conservative rejection) would make the failure mode predictable.
| continue | ||
| if ( | ||
| skip_mesh_pairs | ||
| and self._mesh_manager.get_collision_mesh(child) is not None | ||
| and self._mesh_manager.get_collision_mesh(anchor) is not None | ||
| ): | ||
| continue | ||
| anchor_world_bbox = anchor.get_world_bounding_box().to(device) | ||
| loss = self._no_collision_strategy.compute_loss( | ||
| clearance_m=self.params.clearance_m, | ||
| child_pos=child_pos, | ||
| child_bbox=child_bbox, | ||
| parent_world_bbox=anchor_world_bbox, | ||
| child_obj=child, | ||
| parent_obj=anchor, | ||
| parent_pos=None, | ||
| ) | ||
| if debug: | ||
| print(f" [NoOverlap] {child.name} vs {anchor.name}: loss={loss.mean().item():.6f}") | ||
| total_loss = total_loss + loss | ||
|
|
||
| # Against other non-anchors (unique pairs, both directions) | ||
| for j in range(i + 1, len(non_anchor_objects)): | ||
| other = non_anchor_objects[j] | ||
| if (id(child), id(other)) in on_pairs: | ||
| continue | ||
| if ( | ||
| skip_mesh_pairs | ||
| and self._mesh_manager.get_collision_mesh(child) is not None | ||
| and self._mesh_manager.get_collision_mesh(other) is not None | ||
| ): | ||
| continue |
There was a problem hiding this comment.
_mesh_manager null-dereference when skip_mesh_pairs=True after __deepcopy__
_compute_no_overlap_loss_aabb accesses self._mesh_manager.get_collision_mesh(child) unconditionally when skip_mesh_pairs=True (lines 207 and 231). After __deepcopy__, the custom implementation explicitly sets _mesh_manager = None. Any path that calls _compute_total_loss on a copied solver without first invoking solve() will raise AttributeError: 'NoneType' object has no attribute 'get_collision_mesh'.
A short-circuit guard — if self._mesh_manager is None: skip_mesh_pairs = False or an early-return of zeros — would make post-copy solver state safe.
zhx06
force-pushed
the
zxiao/feature/mesh_support
branch
3 times, most recently
from
923bc46 to
0e57d62
Compare
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
zhx06
force-pushed
the
zxiao/feature/mesh_support
branch
from
0e57d62 to
8c5f342
Compare
2 participants
Summary
Add mesh-based non-collision constraints via sphere-to-SDF, unified with the existing AABB path
Detailed description
CollisionMode.MESHas an alternative to AABB for no-overlap constraints, using greedy sphere decomposition + differentiable Warp SDF queries against actual collision geometry.MeshPairCachedataclass andMeshPairEntryNamedTuple give the mesh path the same collect-then-batch structure as the AABB vectorized path.placer_paramsis the primary configuration source; CLI--collision_mode meshis a fallback.Core files
relations/warp_sdf_kernels.py— differentiable SDF queries on Warp meshesrelations/warp_mesh_manager.py— sphere decomposition and mesh cachingrelations/relation_solver.py— vectorized mesh collision loss during optimizationrelations/mesh_pair_cache.py— typed dataclass for precomputed per-pair collision datarelations/object_placer.py— mesh collision validation at placement time